add: columns to `Eth2Processor` and `BlockProcessor` #6862

agnxsh · 2025-01-20T05:56:29Z

No description provided.

github-actions · 2025-01-20T06:29:04Z

Unit Test Results

      15 files ±0   2 285 suites ±0 1h 9m 59s ⏱️ - 1m 37s
  5 368 tests ±0   5 021 ✔️ ±0 347 💤 ±0 0 ❌ ±0
37 069 runs ±0 36 579 ✔️ ±0 490 💤 ±0 0 ❌ ±0

Results for commit 07ce6ef. ± Comparison against base commit 7a6cc69.

♻️ This comment has been updated with latest results.

beacon_chain/spec/peerdas_helpers.nim

beacon_chain/validators/message_router.nim

tersec · 2025-01-20T18:26:21Z

beacon_chain/validators/message_router.nim

+
+        let kzgCommits =
+          signedBlock.message.body.blob_kzg_commitments.asSeq
+        if dataColumns.len > 0 and kzgCommits.len > 0:


This doesn't seem to actually use kzgCommits though, aside from this check?

(Also, if it's just checking len, it shouldn't need asSeq)

tersec · 2025-01-20T19:28:38Z

beacon_chain/validators/message_router.nim

+      for dc in data_columns:
+        if dc.index in custody_columns:
+          final_columns.add dc
+      dataColumnRefs = Opt.some(final_columns.mapIt(newClone(it)))


Why even create the non-ref final_columns to begin with, only to mapIt immediately?

Once you know you're going to set dataColumnRefs = Opt.some ..., can build it up in place.

beacon_chain/nimbus_beacon_node.nim

beacon_chain/beacon_chain_file.nim

cheatfate · 2025-01-24T12:29:15Z

beacon_chain/beacon_chain_file.nim

+func getDataColumnForkCode(fork: ConsensusFork): uint64 =
+  case fork
+  of ConsensusFork.Fulu:
+    uint64(MaxForksCount)


Because of invalid code range it provides invalid codes which overlaps with blobs range.

resolved in a7af551

cheatfate · 2025-01-24T12:47:11Z

beacon_chain/sync/sync_queue.nim

@@ -132,6 +135,27 @@ proc getShortMap*[T](req: SyncRequest[T],
      res.add('|')
  res

+proc getShortMap*[T](req: SyncRequest[T],
+                     data: openArray[ref DataColumnSidecar]): string =


Looks like this code is based on old and ugly version of blobs map, but it was recently changed to more suitable map format. So the idea is to show how blobs being distributed over the range request, so now it could look like
12.............................., which means that for request range there was returned 1 blob for first block in range and 2 blobs for second one, all other blocks are without blobs, it looks like you still using MAX_BLOBS_PER_BLOCK constant (i'm not sure is it appropriate) but if it so - please change this procedure to new version.

I'm also confused about the usage of MAX_BLOBS_PER_BLOCK. It seems like NUMBER_OF_COLUMNS (https://github.com/ethereum/consensus-specs/blob/v1.5.0-beta.1/specs/fulu/p2p-interface.md#configuration for example) is more relevant.

If NUMBER_OF_COLUMNS* = 128 new short format map should be invented, probably we should use 2 letters for single block, but it definitely should be changed from that version you are using.

beacon_chain/validators/message_router.nim

tersec · 2025-01-25T14:55:57Z

beacon_chain/sync/sync_queue.nim

+                                  data: openArray[Slot]):
+                                  Result[void, cstring] =
+  if data.len == 0:
+    # Impossible to verify empty response


Maybe a nit, I guess? But it's clearly not impossible, since this function does it (affirmatively).

I'd call this something akin to https://en.wikipedia.org/wiki/Vacuous_truth -- maybe vacuously valid.

beacon_chain/sync/sync_queue.nim

beacon_chain/nimbus_beacon_node.nim

beacon_chain/gossip_processing/block_processor.nim

tersec · 2025-01-25T15:55:44Z

beacon_chain/gossip_processing/block_processor.nim

+            MsgSource.gossip, parentBlck.unsafeGet().asSigned(), blobs,
+            Opt.none(DataColumnSidecars))
+
+        var columnsOk = true


nimbus-eth2/beacon_chain/gossip_processing/block_processor.nim

Lines 597 to 639 in 5f169d9

let

parent_root = signedBlock.message.parent_root

parentBlck = dag.getForkedBlock(parent_root)

if parentBlck.isSome():

var blobsOk = true

let blobs =

withBlck(parentBlck.get()):

when consensusFork >= ConsensusFork.Deneb:

var blob_sidecars: BlobSidecars

for i in 0 ..< forkyBlck.message.body.blob_kzg_commitments.len:

let blob = BlobSidecar.new()

if not dag.db.getBlobSidecar(parent_root, i.BlobIndex, blob[]):

blobsOk = false # Pruned, or inconsistent DB

break

blob_sidecars.add blob

Opt.some blob_sidecars

else:

Opt.none BlobSidecars

if blobsOk:

debug "Loaded parent block from storage", parent_root

self[].enqueueBlock(

MsgSource.gossip, parentBlck.unsafeGet().asSigned(), blobs,

Opt.none(DataColumnSidecars))

var columnsOk = true

let columns =

withBlck(parentBlck.get()):

when consensusFork >= ConsensusFork.Fulu:

var data_column_sidecars: DataColumnSidecars

for i in self.dataColumnQuarantine[].custody_columns:

let data_column = DataColumnSidecar.new()

if not dag.db.getDataColumnSidecar(parent_root, i.ColumnIndex, data_column[]):

columnsOk = false

break

data_column_sidecars.add data_column

Opt.some data_column_sidecars

else:

Opt.none DataColumnSidecars

if columnsOk:

debug "Loaded parent block from storage", parent_root

self[].enqueueBlock(

MsgSource.gossip, parentBlck.unsafeGet().asSigned(), Opt.none(BlobSidecars),

columns)

It looks like:

for a pre-Deneb block (still potentially relevant while genesis syncing, for example) or a Deneb/Electra block with no blobs (less common than before, but it happens), the blobsOk loop will be vacuously valid (no blobs means no invalid blobs, and blobsOk is initialized to true), so

nimbus-eth2/beacon_chain/gossip_processing/block_processor.nim

Lines 617 to 619 in 5f169d9

self[].enqueueBlock(

MsgSource.gossip, parentBlck.unsafeGet().asSigned(), blobs,

Opt.none(DataColumnSidecars))

will run, but because the columnsOk-based check separately/independently enqueues blocks and has the same vacuously-valid logic,

nimbus-eth2/beacon_chain/gossip_processing/block_processor.nim

Lines 635 to 637 in 5f169d9

if columnsOk:

debug "Loaded parent block from storage", parent_root

self[].enqueueBlock(

will enqueueBlock(...) the same block again.

In general, I think but haven't enumerated all cases that for the usual valid-blobs-or-columns case (either) the vacuous-truth aspect means that it will typically double-enqueueBlock things (blobs but not columns: the columns check is vacuously true, and the blobs check might be, if the blobs are valid).

Actually, this is even more -- it means that because blocks will have one or the other, all blocks will be either vacuously valid here for either blobs or columns and get enqueued at least once, so this opens a security bypass in block_processor.

beacon_chain/nimbus_beacon_node.nim

beacon_chain/validators/message_router.nim

beacon_chain/sync/sync_manager.nim

tersec · 2025-01-28T09:52:49Z

Run excluded_files="config.yaml|config.nims|beacon_chain.nimble"
The following files do not have an up-to-date copyright year:
- beacon_chain/sync/request_manager.nim
Error: Process completed with exit code 2.

tersec · 2025-01-29T04:01:57Z

beacon_chain/gossip_processing/block_processor.nim

@@ -549,10 +629,28 @@ proc storeBlock(
              Opt.some blob_sidecars
            else:
              Opt.none BlobSidecars
-        if blobsOk:
+        # Blobs and columns can never co-exist in the same block
+        doAssert blobs.isSome and columns.isSome


How does this assert that "Blobs and columns can never co-exist in the same block"?

For this to be the case, exactly one of blobs.isSome and columns.isSome should be true. blobs.isSome and columns.isSome would, rather, imply precisely that a block has both blobs and columns.

resolved, however, again, if you have better suggestions, most welcome

tersec · 2025-01-29T04:04:13Z

beacon_chain/gossip_processing/block_processor.nim

+        # Blobs and columns can never co-exist in the same block
+        doAssert blobs.isSome and columns.isSome
+        # Block has neither blob sidecar nor data column sidecar
+        if blobs.isNone and columns.isNone:


blobs.isSome and columns.isSome implies not-blobs.isNone and columns.isNone. The only way this condition ever holds is when the assertion has already fired/caused a Defect, i.e. the body of this if is unreachable.

this is resolved, however, if you want to do another pass of reviews around here, sure

…Fulu

tersec · 2025-02-05T01:28:25Z

beacon_chain/sync/sync_manager.nim

+  mixin getScore, `==`
+
+  logScope:
+    peer_score = peer.getScore()


There's only one log statement in this function -- not sure logScope is useful?

Also, haven't checked if status-im/nim-chronicles#121 still applies, but at least at one point, logScope didn't work in generic functions, of which this one.

beacon_chain/sync/sync_manager.nim

tersec · 2025-03-03T13:02:44Z

Closed pending reworking/rebasing

agnxsh · 2025-03-03T13:03:00Z

succeeded by #6945

agnxsh marked this pull request as draft January 20, 2025 05:56

tersec reviewed Jan 20, 2025

View reviewed changes

beacon_chain/spec/peerdas_helpers.nim Show resolved Hide resolved

tersec reviewed Jan 20, 2025

View reviewed changes

beacon_chain/spec/peerdas_helpers.nim Show resolved Hide resolved

tersec reviewed Jan 20, 2025

View reviewed changes

beacon_chain/spec/peerdas_helpers.nim Show resolved Hide resolved

tersec reviewed Jan 20, 2025

View reviewed changes

beacon_chain/spec/peerdas_helpers.nim Outdated Show resolved Hide resolved

tersec reviewed Jan 20, 2025

View reviewed changes

beacon_chain/validators/message_router.nim Outdated Show resolved Hide resolved

tersec reviewed Jan 20, 2025

View reviewed changes

tersec reviewed Jan 22, 2025

View reviewed changes

beacon_chain/nimbus_beacon_node.nim Outdated Show resolved Hide resolved

cheatfate reviewed Jan 24, 2025

View reviewed changes

beacon_chain/beacon_chain_file.nim Outdated Show resolved Hide resolved

cheatfate reviewed Jan 24, 2025

View reviewed changes

tersec reviewed Jan 24, 2025

View reviewed changes

beacon_chain/validators/message_router.nim Outdated Show resolved Hide resolved

agnxsh force-pushed the columns branch from 6db36ca to b31706e Compare January 25, 2025 05:24